Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update JR to 3.3.0 snapshot and update test for re-calculation in field list #4845

Merged
merged 2 commits into from
Oct 14, 2021

Conversation

lognaturel
Copy link
Member

@lognaturel lognaturel commented Oct 4, 2021

Update JavaRosa to v3.3.0-SNAPSHOT. getodk/javarosa#646 and getodk/javarosa#644 have been QAed.

I've updated the test that no longer fails with the form from #4170. It's an extension of the problem that the old form displayed -- there's still a calculate being used as a default but then there's also relevance in a field list added to the mix. I'll cherry pick that commit when we have a real PR updating JR. I wanted to give @seadowg a chance to review in the meantime.

Side note, we could likely change the order of relevance update and field list re-rendering so that there wouldn't be an exception in that case. However, similar to the other problems from #4750, the form is nonsense so it's good that there's a crash. As we make more improvements to field lists, it might go away and then this test can be removed.

Initial description for QA NOT FOR MERGING

This is so that @getodk/testers can verify getodk/javarosa#646 and getodk/javarosa#644. As a side effect, the cases described at #4750 should no longer show errors.

Related to the last point, there is now a failing connected test. This is discussed at getodk/javarosa#646 (comment).

@lognaturel lognaturel requested a review from seadowg October 4, 2021 16:59
@lognaturel
Copy link
Member Author

@seadowg please verify that my proposed sequencing is ok with you (prioritizing QA over updating the test).

}
implementation 'net.sf.kxml:kxml2:2.3.0'
implementation 'org.apache.commons:commons-csv:1.4'
implementation files('javarosa-3.2.0.jar')
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still 3.2.0 because it's off the unmerged getodk/javarosa#646 but it's the release candidate for 3.3, basically.

@seadowg
Copy link
Member

seadowg commented Oct 4, 2021

@lognaturel yeah seems good! I'll add "needs testing" to this. I'll also just make it a draft so it's even clearer it's not for merging.

@seadowg seadowg marked this pull request as draft October 4, 2021 17:02
@seadowg seadowg removed their request for review October 4, 2021 17:24
@lognaturel
Copy link
Member Author

I thought the build failure might have been something intermittent but was wrong. I assume it has to do with including the jar.

@seadowg
Copy link
Member

seadowg commented Oct 5, 2021

@lognaturel definitely could be. @mmarciniak90 @kkrawczyk123 this is still good to test even though the build is failing. We'll sort all that out when we've merged the JavaRosa PR.

.clickOnText("A")
.clickOnForm("Relevance and calculate loop")
.answerQuestion(1, "B")
.scrollToAndAssertText("third")
Copy link
Member Author

@lognaturel lognaturel Oct 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels awkward but I couldn't find a better way to make sure that the third question was displayed on a short screen. Scrolling to the question text is insufficient because the edittext needs to be on-screen.

@@ -171,11 +171,6 @@ public FormEntryPage clickRankingButton() {
return this;
}

public FormEntryPage putTextOnIndex(int index, String text) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved and renamed this because it took me a long time to find it.

@mmarciniak90
Copy link
Contributor

Tested with success

Verified on Android 8.1, 9.0, 10.0

Verified cases:

  • filled and edited forms
  • jumped to questions via hierarchy view on new and saved form
  • added and removed groups
  • no crash in crashOnFieldList form but the error is still visible
  • all questions can be filled in minimal-select-field-list form and error does not appear
  • regression check on all mentioned forms
  • compared the behavior with v2021.2.0
  • light/dark mode

Verified forms from project 340

  • cascade
  • crashOnFieldList
  • dynamiChoices
  • External 52k split
  • minimal-select-field-list
  • Nigeria Wards Internal
  • Relevance and calculate loop
  • select-one-file

@kkrawczyk123
Copy link
Contributor

Verified also on Androids 5.1 and 11.

@seadowg
Copy link
Member

seadowg commented Oct 13, 2021

@lognaturel I think we can do a snapshot and then upgrade Collect to that now as part of this PR.

@lognaturel
Copy link
Member Author

Thanks, @kkrawczyk123, @mmarciniak90!

@seadowg did you review the test change? I’ll update the first commit in the next 24hrs to use the snapshot but you could look at the test if you haven’t already.

@seadowg
Copy link
Member

seadowg commented Oct 13, 2021

@lognaturel ah sorry I thought you hadn't got round to that because of the failures but looks like those are just mem issues. Will set myself as a reviewer. Circle CI won't let me rerun annoyingly as it's your PR!

@lognaturel lognaturel changed the title Update JR for QA of better CSV parsing and answer binding on form re-open Update JR to 3.3.0 snapshot and update test for re-calculation in field list Oct 14, 2021
@lognaturel lognaturel marked this pull request as ready for review October 14, 2021 03:56
@seadowg seadowg merged commit cf2fdac into getodk:master Oct 14, 2021
@lognaturel lognaturel deleted the jr-3.3 branch October 14, 2021 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants